Conversation
|
Thanks for the contribution!
中文感谢你的贡献！
|
|
Claude finished @xinli-sw's task in 2m 51s —— View job Review of PR #3394
LGTM - no blocking issues found. Checks that passed by inspection:
Not verified in this run: the sandbox blocked network access (and the vLLM source MCP server timed out), so I could not confirm that |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because configs/nvidia-master.yaml is CODEOWNERS-protected and this change pins a new vLLM nightly image together with new attention/KV-cache flags on the launch path, a human (owner) look would still be worthwhile.
What was reviewed:
- Traced
EVAL_ONLY/VLLM_ENGINE_READY_TIMEOUT_Sthroughbenchmarks/runtime_settings.sh,runners/launch_b300-dsxe.sh, and the GitHub workflow templates — every caller path already supplies both explicitly, so the newcheck_env_varscalls won't hard-fail. - Verified the new TP2/TP4
--attention-configbranch againstconfigs/nvidia-master.yaml's search-space, which only definestp: 2andtp: 4for this recipe. - Checked the new
indexer_kv_dtype":"mxfp4"/indexer_sparse_logitssettings against CONTRIBUTING.md's draft-precision rule — these configure the target model's sparse-attention indexer (long-context KV selection), not the DSpark draft model, so that rule doesn't apply. - Confirmed the
perf-changelog.yamlentry is append-only and matches the described change.
Extended reasoning...
The diff bumps a pinned vLLM image tag, adds a TP-conditional --attention-config and --kv-cache-dtype fp8 to one benchmark launch script, hardens two env vars (EVAL_ONLY, VLLM_ENGINE_READY_TIMEOUT_S) to required check_env_vars instead of inline defaults, and appends a matching perf-changelog entry. It touches no auth, crypto, or data-exposure surface; this is pure benchmark/launch configuration. I independently traced the env-var propagation across all runner and workflow callers and confirmed no path would hit the new hard-fail, and checked the mxfp4/sparse-logits additions against the repo's draft-precision policy (they belong to the target model's sparse attention indexer, not the DSpark draft, so no violation). The deciding factor for defer over approve is that configs/nvidia-master.yaml falls under CODEOWNERS (@ ankur-singh, @ kedarpotdar-nv, @ SemiAnalysisAI/core), so a human owner sign-off is still warranted despite no findings.
|
View unofficial run (performance): https://inferencex.semianalysis.com/inference?unofficialRun=36084679196 View unofficial run (accuracy): https://inferencex.semianalysis.com/evaluation?unofficialRun=36084679196 |
fb0c79c to
9a6570b
Compare
|
Claude finished @xinli-sw's task in 2m 13s —— View job Review of PR #3394 (ready for review)
LGTM - no blocking issues found. This is the same commit the two earlier Claude passes reviewed, so I am not repeating that feedback. Two things are new since then. Sweep status. The first sweep (run 36065953173) finished and published unofficial-run links, and a fresh sweep (run 36066605303) started when the PR went ready for review. I could not read per-job conclusions from this sandbox, so please confirm both TP2 and TP4 arms are green in the new run before merge. Description vs. diff. The PR text says the change preserves "the existing topology and memory limits", but the diff also drops the explicit Verified locally:
Not verified (network blocked, vLLM source MCP server timed out): that |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I checked two other candidates around the new CAPTURE_SIZE doubling loop: a non-positive DSV41_MIN_CUDAGRAPH_CAPTURE_SIZE spinning forever, and the TP2/CONC=128 case where the 512 cap sits below CONC*(1+NUM_SPEC_TOKENS). Both mirror the pre-existing doubling-loop and 512 cap already used in benchmarks/single_node/agentic/dsv41flash_fp4_vllm_mtp.sh, so neither is a risk newly introduced by this change.
Extended reasoning...
This PR pins a new vLLM nightly and reworks CUDA-graph capture sizing plus attention-backend/KV-dtype selection in the B300 DeepSeek-V4.1-Flash vLLM benchmark script, adds a launcher env export, and appends a perf-changelog entry; it touches no auth/crypto/security surface. The confirmed inline finding is a stale doc comment in configs/nvidia-master.yaml. Two other candidate issues (an env-var-driven infinite loop and an undersized CUDA-graph cap at TP2/high concurrency) were investigated and ruled out because they replicate an already-shipped pattern in the sibling dsv41flash_fp4_vllm_mtp.sh script rather than being newly introduced.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
configs/nvidia-master.yaml— nit: the comment at configs/nvidia-master.yaml:8429-8430 ("The B300 script uses explicit piecewise CUDA graph capture tiers") is now stale. The diff replaced the piecewise GRAPH_SIZES/COMPILATION_CONFIG list in dsv41flash_fp4_b300_vllm_mtp.sh with a single doubling CAPTURE_SIZE, so there are no more capture tiers. Fix: update or remove the comment so it matches the new CAPTURE_SIZE logic.Why this was flagged
configs/nvidia-master.yaml:8430 still says the B300 script uses explicit piecewise CUDA graph capture tiers, but benchmarks/single_node/agentic/dsv41flash_fp4_b300_vllm_mtp.sh no longer has GRAPH_SIZES/COMPILATION_CONFIG (removed by this diff) and instead derives a single CAPTURE_SIZE via a doubling loop. A maintainer reading the config comment gets a false description of the current script behavior. This is a documentation-only nit, not a functional bug.
Verification: nit: The comment at configs/nvidia-master.yaml:8429-8430 ("The B300 script uses explicit piecewise CUDA graph capture tiers.") is now stale. This diff removed the piecewise machinery from benchmarks/single_node/agentic/dsv41flash_fp4_b300_vllm_mtp.sh — the diff deletes
GRAPH_SIZES_2046='6,12,18,...,2046',GRAPH_SIZES_8190,CAPTURE_SIZE="${GRAPH_SIZES##*,}",… | nit: The comment at…
2859ea9 to
83b8629
Compare
更新 B300 DeepSeek-V4.1-Flash vLLM nightly 配方，恢复原有 CUDA graph 与批处理分档，并在 TP2 和 TP4 使用 FlashInfer 稀疏注意力。
83b8629 to
fcfefcd
Compare
|
InferenceX has switched away from unmaintainable bash scripts to YAML files that don't repeat the same stuff over and over again. Please merge the latest |
…LLM nightly-0961bbae with FlashInfer sparse indexer + fp8 KV Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Closing in favor of #3458, which ports this PR onto the srt-slurm recipe that #3428 switched |
2 participants
Pin the B300 DeepSeek-V4.1-Flash AgentX arm to vLLM nightly
0961bbae. Enable the merged Blackwell sparse indexer settings and select FlashInfer at TP2, MegaAttention at TP4. Preserve the existing topology and memory limits. Also pass the engine readiness timeout from the launcher where needed.The vLLM tracking issue and upstream recipe document the included changes. Local shell, YAML, and exact-key matrix checks pass; GPU sweep and eval are pending.
中文
将 B300 DeepSeek-V4.1-Flash AgentX 配置固定到 vLLM nightly
0961bbae。启用已合并的 Blackwell 稀疏索引器参数，TP2 选择 FlashInfer，TP4 选择 MegaAttention；保留现有拓扑与显存限制，并由启动脚本传入所需的引擎就绪超时。相关改动见 vLLM 跟踪问题 与上游配方。本地 shell、YAML 和指定配置的矩阵检查通过；GPU sweep 与 eval 待运行。